[TLS 1.3] Codepoints for ECDH w/ Brainpool (RFC 8734) #3810

reneme · 2023-11-17T11:22:23Z

IETF deprecated the code points for the brainpool curves that were used in TLS 1.2 and earlier. Those were initially defined in RFC 7027 (26-28). Therefore, for TLS 1.3 new code points were proposed in RFC 8734 (31-33). 😓

This basically introduced the new code points as aliases for the old ones (similarly to what we've done with some yet-to-be-defined code points for some hybrid algorithms). Technically, for TLS 1.3 handshakes we should ensure that the new ones are used and vice versa. However, I think, for interoperability's sake, we should just let the user configure whatever combination they want.

Also, I added TLS::Group_Params::usable_in_version(Protocol_Version) that returns false when the key exchange group is not compatible with the version parameter. Currently, that filters out the described brainpool constellations and also hybrid key exchanges in TLS 1.2. However, the filter is applied only when creating Client Hello messages. I.e. if we send a Client Hello that insists on TLS 1.3, only the new brainpool code points will be advertised and vice versa. A Client Hello that allows both protocol version may also advertise both brainpool code point families.

I'm open for better suggestions, of course.

coveralls · 2023-11-17T12:41:14Z

coverage: 92.043% (-0.02%) from 92.06%
when pulling 837453c on Rohde-Schwarz:tls13/brainpool_for_kex
into 20279c6 on randombit:master.

securitykernel

This is really unfortunate and unnecessarily complicates things. Just one comment on the curve string to group params matching.

securitykernel · 2023-11-18T11:43:55Z

src/tests/data/tls-policy/bsi.txt

@@ -10,7 +10,7 @@ signature_hashes = SHA-512 SHA-384 SHA-256
 macs = AEAD SHA-384 SHA-256
 key_exchange_methods = ECDH DH ECDHE_PSK
 signature_methods = ECDSA RSA DSA
-key_exchange_groups = brainpool512r1 brainpool384r1 brainpool256r1 secp521r1 secp384r1 secp256r1 ffdhe/ietf/4096 ffdhe/ietf/3072


IMHO the policy files should not be touched by this change. Users may find it confusing that brainpool curves are present twice here each, I think it would become a common source for error. "brainpool512r1" should be matched by the code to the corresponding code point automatically, just as Group_Params::to_algorithm_spec() does the other way around.

I fully agree on the potential end user confusion. Though, I'm somewhat on the fence whether users may still need the additional flexibility in configuration that it brings.

Just to make sure we're on the same page: You're suggesting, that brainpool384r1 (for instance) should translate into advertising both code points (and potentially offering key share values for both in a TLS 1.3 ClientHello), right?

On the one hand, this would somewhat complicate the logic of the code point selection and introduce a potential source of bugs (as we will likely need to handle this Brainpool-specific special case in more than one place). On the other hand, it would prevent users from explicitly choosing one code point over the other.

This is explicitly not meant to be an objection, though. However, if we introduce the additional logic, I want to suggest a compromise: Let's add the following configuration options:

brainpool*r1 (the existing name) - add both code points

brainpool*r1_legacy - add the old code points only

brainpool*r1_tls13 - add the new code points only

That approach would also make the "special case" more explicit in the code base. The existing name could map to some special Group_Params_Code::BRAINPOOL_META_VALUE which we can handle explicitly in a special case (and map to ::BRAINPOOL_TLS12 and ::BRAINPOOL_TLS13).

Whatever we do: its a mess. 😢

randombit · 2023-12-02T22:05:56Z

What a totally unnecessary mess this is. Alas.

One concern I have here is that we can't quite treat these two points as equivalent. In particular, IIUC, technically speaking if a client sends a 1.2+1.3 client hello that contains just a 1.2 brainpool curve ID, we must not use brainpool with 1.3.

I'm not sure what the best fix is.

One interesting "out" is that afaik RFC 8734 does not prohibit using the 1.3 brainpool IDs in TLS 1.2 also. But that will cause interop problems with stacks which only support the 1.2 IDs.

securitykernel · 2023-12-03T14:05:56Z

You're suggesting, that brainpool384r1 (for instance) should translate into advertising both code points (and potentially offering key share values for both in a TLS 1.3 ClientHello), right?

What I think would be the best for users is to avoid having them to deal with the fact that there are two points at all. For the user, the only selection he should make is between the three brainpool*r1 -- in policy files as well as programatically. Botan should then figure out which code point to advertise. This puts the burden on the library's shoulders, I see.

securitykernel · 2023-12-03T14:06:31Z

I just looked at how OpenSSL solved this mess, and it seems they first implemented the approach I suggest, and then later refactored to what essentially this PR implements.

reneme · 2023-12-05T13:22:37Z

FWIW: We tested this with OpenSSL 3.2.0 and it works with both TLS 1.2 and 1.3.

Botan TLS policy:

allow_tls13 = true
allow_tls12 = true
allow_dtls10 = false
allow_dtls12 = false
key_exchange_groups = brainpool256r1tls13 brainpool256r1

OpenSSL CLI commands:

# TLS 1.3
openssl s_client -connect localhost:50447 -debug -curves brainpoolP256r1tls13:brainpoolP256r1:secp256r1 -tls1_3

# TLS 1.2
openssl s_client -connect localhost:50447 -debug -curves brainpoolP256r1tls13:brainpoolP256r1:secp256r1 -tls1_2

randombit · 2023-12-05T14:31:45Z

What happens with these

openssl s_client -connect localhost:50447 -debug -curves brainpoolP256r1:secp256r1 -tls1_3

openssl s_client -connect localhost:50447 -debug -curves brainpoolP256r1tls13:secp256r1 -tls1_2

reneme · 2023-12-06T08:34:39Z

Those cases don't result in a successful handshake. OpenSSL doesn't seem to offer the groups cross-version. Perhaps we should also try with OpenSSL as the server. Whether they accept TLS 1.3 with the legacy ID and vice versa.

`-curves brainpoolP256r1:brainpoolP256r1tls13:secp256r1`

When no TLS version restriction is given, tls13 is offered but non-TLS1.3 is marked as supported.

Key Shares: brainpool256r1tls13
Supported Groups: brainpool256r1 brainpool256r1tls13 secp256r1

`-curves brainpoolP256r1:secp256r1 -tls1_3`

Cross-protocol groups are not offered.

Key Shares: secp256r1
Supported Groups: secp256r1

`-curves brainpoolP256r1tls13:secp256r1 -tls1_2`

Supported Groups: secp256r1

randombit · 2023-12-06T14:06:16Z

Those cases don't result in a successful handshake.

By not successful do you mean the handshake ends up using P256? Or an alert occurs?

reneme · 2023-12-06T15:27:11Z

By not successful do you mean the handshake ends up using P256? Or an alert occurs?

In my particular example, it failed with "no common group". But I configured the botan-based server to exclusively support brainpool. Otherwise, it would just have used P256.

This was required because the brainpool curves have two distinct sets of code points. For TLS 1.2 (and earlier) they are defined in RFC 7027 and for TLS 1.3 in RFC 8734.

reneme · 2023-12-07T14:39:16Z

Perhaps we should also try with OpenSSL as the server.

Turns out that an OpenSSL server won't negotiate a connection with TLS 1.3 and the legacy code points either.

I tested with a Botan client that is configured to insist on TLS 1.3 but that offers the legacy brainpool code points only. The OpenSSL server is configured to allow both TLS 1.2 and 1.3 and supports both the new and old brainpool code points. This server rejects the handshake with "no suitable key share".

FWIW: OpenSSL prior to 3.2.0 -- that doesn't support the new code points yet -- also rejects negotiating brainpool (using the old code points) in TLS 1.3.

reneme · 2023-12-07T15:13:30Z

For the user, the only selection he should make is between the three brainpool*r1 -- in policy files as well as programatically.

Yeah, I agree, that would be great from a user's perspective. And the experiments with OpenSSL above also support such an approach. I'll see tomorrow, if there's a reasonable way to implement it like this. I'm hoping to centralize the necessary mappings somehow. If we have to scatter them across the TLS implementation(s), it's too big of a mess with lots of room for future fuck-up, all for too little user benefit, in my opinion.

reneme · 2023-12-08T12:53:36Z

The more I look at that, the worse it gets, frankly. Handling all the edge cases properly, requires special stuff in several locations of the code base. Those special cases are also non-trivial to test. With our current test infrastructure we would probably end up building end-to-end tests against OpenSSL 3.2+.

When trying to build this as an abstract policy, one needs to mentally mix in hybrid key exchange algorithms, which we currently support in TLS 1.3 but not 1.2. I.e. that must not be offered when also offering TLS 1.2. In contrast to brainpool, where just the offered code point changes contextually.

What @securitykernel suggested is neat for sure: i.e. hide this complexity from the user by transparently juggling the right code points depending on the available protocol version. Though, that's a completely new concept (i.e. contextually mapping one NamedGroup to one, the other or both code points, and vice versa) in our policy architecture on top of the whole mess this already is.

As an illustration, here are some combinations to keep in mind when implementing this one way or the other:

Client
- Offering just TLS 1.2, just 1.3 and both (allowing for downgrade)
- With new, old and both code points
- Check that we abort if the peer mixes 1.3 and the old points, and vice versa
- (probably more)
Server
- Allow for just TLS 1.2, just 1.3 and both (allowing for downgrade)
- Ensure that selected protocol version also results in correct code point revision
  - That might include a Hello Retry Request if a client offers a key share with the old points but allows for usage of TLS 1.3
- Check that we abort if the peer mixes 1.3 and the old points, and vice versa
- (probably much more)

All of that extra plumbing for the slim use case that Brainpool is at the end of the day.

Long story short, I'd like to suggest to KISS! Let's introduce the new code points and don't do any additional handling or validation. That will result in Botan allowing to mix and match protocol version and code point revisions. And this might result in interop issues with other implementations. But it gives the library user full flexibility to work around interop issues for their specific use case.

@securitykernel @randombit What do you think?

reneme added the enhancement Enhancement or new feature label Nov 17, 2023

reneme added this to the Botan 3.3.0 milestone Nov 17, 2023

reneme requested a review from securitykernel November 17, 2023 11:22

reneme self-assigned this Nov 17, 2023

reneme force-pushed the tls13/brainpool_for_kex branch from c63a9aa to c16b9cb Compare November 17, 2023 12:19

reneme force-pushed the tls13/brainpool_for_kex branch from c16b9cb to 837453c Compare November 17, 2023 14:09

securitykernel requested changes Nov 18, 2023

View reviewed changes

randombit self-requested a review November 18, 2023 23:52

reneme mentioned this pull request Dec 6, 2023

[TLS 1.3] CLI: ./botan tls_client_hello can deal with TLS 1.3 #3820

Merged

Add TLS::Group_Params::to_algorithm_spec()

de386d5

This was required because the brainpool curves have two distinct sets of code points. For TLS 1.2 (and earlier) they are defined in RFC 7027 and for TLS 1.3 in RFC 8734.

reneme added 2 commits December 7, 2023 15:40

TLS::Group_Params::usable_in_version()

64f1203

introduce TLS 1.3 specific code points for brainpool (RFC 8734)

72a9810

reneme force-pushed the tls13/brainpool_for_kex branch from 837453c to 72a9810 Compare December 7, 2023 14:41

reneme modified the milestones: Botan 3.3.0, Botan 3.4.0 Feb 13, 2024

reneme modified the milestones: Botan 3.4.0, Botan 3.5.0 Apr 8, 2024

reneme removed this from the Botan 3.5.0 milestone Jul 2, 2024

reneme added this to the Botan 3.6.0 milestone Jul 2, 2024

reneme modified the milestones: Botan 3.6.0, Botan 3.7.0 Oct 7, 2024

reneme mentioned this pull request Nov 14, 2024

TLS SignatureScheme lacks support for Brainpool #3811

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TLS 1.3] Codepoints for ECDH w/ Brainpool (RFC 8734) #3810

[TLS 1.3] Codepoints for ECDH w/ Brainpool (RFC 8734) #3810

reneme commented Nov 17, 2023 •

edited

Loading

coveralls commented Nov 17, 2023 •

edited

Loading

securitykernel left a comment

securitykernel Nov 18, 2023

reneme Nov 20, 2023

randombit commented Dec 2, 2023

securitykernel commented Dec 3, 2023 •

edited

Loading

securitykernel commented Dec 3, 2023

reneme commented Dec 5, 2023

randombit commented Dec 5, 2023

reneme commented Dec 6, 2023

randombit commented Dec 6, 2023

reneme commented Dec 6, 2023

reneme commented Dec 7, 2023 •

edited

Loading

reneme commented Dec 7, 2023

reneme commented Dec 8, 2023

[TLS 1.3] Codepoints for ECDH w/ Brainpool (RFC 8734) #3810

Are you sure you want to change the base?

[TLS 1.3] Codepoints for ECDH w/ Brainpool (RFC 8734) #3810

Conversation

reneme commented Nov 17, 2023 • edited Loading

coveralls commented Nov 17, 2023 • edited Loading

securitykernel left a comment

Choose a reason for hiding this comment

securitykernel Nov 18, 2023

Choose a reason for hiding this comment

reneme Nov 20, 2023

Choose a reason for hiding this comment

randombit commented Dec 2, 2023

securitykernel commented Dec 3, 2023 • edited Loading

securitykernel commented Dec 3, 2023

reneme commented Dec 5, 2023

randombit commented Dec 5, 2023

reneme commented Dec 6, 2023

-curves brainpoolP256r1:brainpoolP256r1tls13:secp256r1

-curves brainpoolP256r1:secp256r1 -tls1_3

-curves brainpoolP256r1tls13:secp256r1 -tls1_2

randombit commented Dec 6, 2023

reneme commented Dec 6, 2023

reneme commented Dec 7, 2023 • edited Loading

reneme commented Dec 7, 2023

reneme commented Dec 8, 2023

reneme commented Nov 17, 2023 •

edited

Loading

coveralls commented Nov 17, 2023 •

edited

Loading

securitykernel commented Dec 3, 2023 •

edited

Loading

`-curves brainpoolP256r1:brainpoolP256r1tls13:secp256r1`

`-curves brainpoolP256r1:secp256r1 -tls1_3`

`-curves brainpoolP256r1tls13:secp256r1 -tls1_2`

reneme commented Dec 7, 2023 •

edited

Loading